Story 2422: Sign In Webpage Integration#2487
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Besides the redirect bug, could we also strip out all legacy stuff?
That means:
- Removing the
V3LoginViewfromviews.py; - Removing
context["login_url"] = reverse_lazy("v3-login")fromcore/mixins.py; - Removing the path
v3/accounts/login/fromconfig/v3_urls.py;
| class CustomLoginView(LoginView): | ||
| class CustomLoginView(V3AuthContextMixin, LoginView): | ||
| v3_template_name = "v3/accounts/login.html" | ||
|
|
There was a problem hiding this comment.
When the V3Mixing dispatches, it sets _v3_active = False for POST requests, so it falls through the legacy login page. That means, if an error occurs during login, we're redirected to the legacy page.
To reproduce:
- Go to login page;
- Insert an email;
- Insert a wrong password and submit;
There was a problem hiding this comment.
This should be fixed with changes made in the base branch, to better handle V3 integrations
jlchilders11
force-pushed
the
jc/webpage-integration-sign-in
branch
from
ae386f0 to
0b24e9a
Compare
jlchilders11
force-pushed
the
jc/webpage-integration-sign-in
branch
from
0b24e9a to
5b6922a
Compare
julioest
left a comment
julioest
left a comment
There was a problem hiding this comment.
Approving! The login page changes are solid: confirmed the mixin is wired and the hidden redirect plumbing is correct
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
This looks great to me! I'll hold off from approving it at the moment since this PR still need a rebase to pull in the Sign Up changes. Can loop back whenever this is ready! 🙌
There was a problem hiding this comment.
Heya, look great to me, and it's working well regardless of JS presence.
About the social sign in, I think we can at least have it implemented as we think it's the correct way so we can test them in QA before formal approval. I've created a separate ticket for this though, so we don't drag this PRs flow (#2507).
jlchilders11
force-pushed
the
jc/webpage-integration-sign-in
branch
from
5b6922a to
561f3bc
Compare
This should now be rebased onto the sign up page changes! |
4 participants
Issue: #2422
Summary & Context
Implements the V3 version of the sign in page. Note that this branches off of: #2482 to take advantage of the changes made to auth pages in general.
Changes
Please list any potential risks or areas that need extra attention during review/testing
There is no great way to test the social sign in locally, since it relies on Oauth that is not set up for development.
Screenshots
Self-review Checklist
Frontend